Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: add number-isnan eslint rule #17556

Closed
wants to merge 2 commits into from
Closed

tools: add number-isnan eslint rule #17556

wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Dec 8, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tools, test

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 8, 2017
@apapirovski
Copy link
Member

This might be a bit finicky since it would catch const isNaN = Number.isNaN; isNaN(val); too? But maybe that's not a real problem... 😆

@Trott
Copy link
Member

Trott commented Dec 8, 2017

No objections, but one question and one observation.

Question: What's the reason for avoiding the isNaN() global?

Observation: I think this can probably be implemented using no-restricted-syntax rather than a custom rule.

@maclover7
Copy link
Contributor Author

@Trott

Question: What's the reason for avoiding the isNaN() global?

For personal reasons, I think it's more clear to use Number.isNaN. Also, several Code and Learn PRs have been submitted to move towards Number.isNaN, so I figured that people had come to consensus that it was a good idea.

Observation: I think this can probably be implemented using no-restricted-syntax rather than a custom rule.

Interesting -- refactored down to use no-restricted-syntax

apapirovski
apapirovski reviewed Dec 9, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this won't work as it overrides all the no-restricted-syntax rules declared within .eslintrc.yaml in the root directory.

@Trott
Copy link
Member

Trott commented Dec 10, 2017

Unfortunately this won't work as it overrides all the no-restricted-syntax rules declared within .eslintrc.yaml in the root directory.

Oh, yeah, I assumed it was a lint rule we'd want for the entire code base...

@apapirovski
Copy link
Member

Oh, yeah, I assumed it was a lint rule we'd want for the entire code base...

Yeah, I think we could do that? I don't see any reason why we would be stricter about this in test than lib.

@Trott
Copy link
Member

Trott commented Dec 10, 2017

Are we sure we want to necessarily encourage people to change isNaN() to Number.isNan(). They behave differently.

isNaN('a string is not a nubmer'); //true
Number.isNaN('a string is not a number'); // false

@apapirovski
Copy link
Member

Number.isNaN should be the new standard way of doing this as per ES2015. There's weird coercion in isNaN.

@maclover7
Copy link
Contributor Author

updated @apapirovski @Trott

For context, I was initially hesitant to add the rule to lib/, since we are generally performance sensitive, but I don't think the change will massively alter performance. Should benchmarks be run anyways?

@Trott
Copy link
Member

Trott commented Dec 11, 2017

@maclover7 Sorry to make this tedious, but this is what I'd recommend:

  • Revert to your initial custom rule and apply it just to test
  • If you wish, in a separate PR, propose changing isNaN() to Number.isNaN() in lib and prepare to run benchmarks and have a fair amount of conversation about it. :-D
  • If the above bullet point suggestion lands, then use the restricted-syntax thing to replace your custom rule.

@Trott
Copy link
Member

Trott commented Dec 11, 2017

(And yeah, had I not said anything, first bullet point would have happened and all of this would have been avoided. Sorry!)

not-an-aardvark
not-an-aardvark reviewed Dec 13, 2017
View reviewed changes
lib/events.js Outdated
@@ -75,7 +75,7 @@ EventEmitter.init = function() {
// Obviously not all Emitters should be limited to 10. This function allows
// that to be increased. Set to zero for unlimited.
EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) {
if (typeof n !== 'number' || n < 0 || isNaN(n)) {
if (typeof n !== 'number' || n < 0 || Number.isNaN(n)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typeof n !== 'number' check is redundant if Number.isNaN is being used.

lib/readline.js Outdated
@@ -105,7 +105,7 @@ function Interface(input, output, completer, terminal) {
}

if (typeof historySize !== 'number' ||
isNaN(historySize) ||
Number.isNaN(historySize) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typeof historySize !== 'number' check is redundant if Number.isNaN is being used.

@maclover7
Copy link
Contributor Author

updated @Trott, PTAL

@maclover7
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/12184/

@maclover7
Copy link
Contributor Author

Will land tomorrow unless any objections

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2017
@maclover7 maclover7 self-assigned this Dec 19, 2017
@maclover7
Copy link
Contributor Author

Landed in e554bc8

@maclover7 maclover7 closed this Dec 19, 2017
@maclover7 maclover7 deleted the jm-number-isnan branch December 19, 2017 18:22
maclover7 added a commit to maclover7/node that referenced this pull request Dec 19, 2017
@maclover7
tools: add number-isnan rule
e554bc8 
PR-URL: nodejs#17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@maclover7 @MylesBorins
tools: add number-isnan rule
e7a99b3 
PR-URL: #17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@maclover7 @MylesBorins
tools: add number-isnan rule
6531401 
PR-URL: #17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@maclover7 @MylesBorins
tools: add number-isnan rule
a84d145 
PR-URL: #17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@maclover7 @gibfahn
tools: add number-isnan rule
0e37054 
PR-URL: #17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@maclover7 @MylesBorins
tools: add number-isnan rule
4879038 
PR-URL: #17556
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Feb 27, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-an-aardvark not-an-aardvark not-an-aardvark left review comments

@apapirovski apapirovski apapirovski left review comments

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

Assignees

@maclover7 maclover7

Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@maclover7 @apapirovski @Trott @cjihrig @not-an-aardvark @MylesBorins @addaleax @nodejs-github-bot